[11.x] Ensure batched jobs are actually batchable #54442
Conversation
|
Looks like I missed a few things in this implementation. I'm looking into it... Converted the PR to draft, in the meantime. Edit: I think we're good for review, now. |
src/Illuminate/Bus/PendingBatch.php
Outdated
| return; | ||
| } | ||
|
|
||
| if (! in_array(Batchable::class, class_uses_recursive($job))) { |
There was a problem hiding this comment.
I wonder if it wouldn't be useful to cache the job's class so you don't have to call class_uses_recursive for multiple instances of the same class.
There was a problem hiding this comment.
While it's not possible with class_uses_recursive, this could be taken one step further, by implementing something similar to class_uses_recursive: Caching all classes in the recursive chain 🤔
|
There might be cases, even though, it might be rare, where the trait isn't even used for some reason, but the functionality is implemented. This wouldn't work anymore. This could possibly be avoided by using an interface 🤔 |
|
This PR has unnecessary doc block changes. |
Reverted those automated changes. |
|
What about batch callbacks? |
5 participants
Although the documentation for batch jobs clearly states that batched jobs should use the
Batchabletrait, there's not much that alerts or prevents the dev from wrongly trying to batch jobs that don't implement it, resulting in runtime problems. Even if we use theassertBatchedtest, tests pass even with invalid jobs supplied. I was a victim of this. (I know, it might also be a skill issue from my side, for forgetting to use the trait...)This PR adds a check to batched jobs to ensure that the jobs use the required
Batchabletrait, throwing aRuntimeExceptionif a job is found in a given list when callingBus::batchor if supplied to theaddmethod of aPendingBatchinstance.